Support ClassData and TokenData not according to ICS-721 spec #1039

yito88 · 2024-01-16T20:43:10Z

Description

Custom (de)serialization for ClassUri, ClassData, TokenUris, and TokenData to decode null
~~Handle Data as just String if ics721-data feature flag isn't set~~ Allowing to check if the Data is in ICS-721 structure
- Currently, go implementation doesn't care about the JSON structure in the data, e.g. cw-ics721 implementation uses its own structure
- (I have some uncertainty about whether using a feature flag is the optimal approach for toggling the data. If you have any alternative suggestions or insights, I would greatly appreciate your input.)
Change functions and some arguments for ClassData and TokenData to use Option because URI and data (ClassData, TokenData) could not exist
Check the length of TokenUris and TokenData in send_nft_transfer_validate()

PR author checklist:

Added changelog entry, using unclog.
Added tests.
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).
Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

codecov · 2024-01-16T20:46:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (feat/ics721-impl@3c0fddd). Click here to learn what that means.

Additional details and impacted files

@@                 Coverage Diff                 @@
##             feat/ics721-impl    #1039   +/-   ##
===================================================
  Coverage                    ?   67.25%           
===================================================
  Files                       ?      196           
  Lines                       ?    20275           
  Branches                    ?        0           
===================================================
  Hits                        ?    13635           
  Misses                      ?     6640           
  Partials                    ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Farhad-Shabani

Thank you @yito88 for tracking down the Data format issue. 🙏🏻

Handle Data as just String if ics721-data feature flag isn't set

I could come up with another approach for handling any Data format, allowing us to check if the Data is in ICS-721 structure. We shouldn't need to define a feature flag.
Please check out this branch and apply if that makes sense to you. I couldn’t open another PR to your branch.

Change functions and some arguments for ClassData and TokenData to use Option because URI and data (ClassData, TokenData) could not exist

Shouldn't the get_* methods under the NftContext and NftClassContext always return some values? Is it viable for instance we mint a token by only having a pair of class and token ids without any URIs and data?

Similar question applies to the create_or_update_class_execute and mint_nft_execute methods. Do you handle e.g. the TokenUri and TokenData creation locally as the host specifies, or won't you need them at all?

Check the length of TokenUris and TokenData in send_nft_transfer_validate()

Let's define a validate_basic() method for the PacketData so can call that in places we need similar checks. I've showcased that in this branch.

yito88 · 2024-01-18T16:24:42Z

@Farhad-Shabani Thank you! I merged your commits for the data format.
I need to clarify the ICS-721 structure (In a discussion, the structure might be just a recommended one). However, the check function will be helpful.

Shouldn't the get_* methods under the NftContext and NftClassContext always return some values? Is it viable for instance we mint a token by only having a pair of class and token ids without any URIs and data?

Similar question applies to the create_or_update_class_execute and mint_nft_execute methods. Do you handle e.g. the TokenUri and TokenData creation locally as the host specifies, or won't you need them at all?

Technically, we can mint a token only with class and token IDs. In my testing, some optional data were empty. I'm testing actual chains.

Farhad-Shabani · 2024-01-18T20:04:41Z

I need to clarify the ICS-721 structure (In a discussion, the structure might be just a recommended one). However, the check function will be helpful.

That's appreciated. Thus, I will be waiting for your confirmation before we proceed with this PR.

Technically, we can mint a token only with class and token IDs. In my testing, some optional data were empty. I'm testing actual chains.

Thanks! Good to know this. Let me know plz the actual test results as well.

Farhad-Shabani

Just left a comment and please fix the CI catch with the cargo doc, then good to go. 🎉 Thank you @yito88 for implemeting the custom (de)serialization.

Farhad-Shabani · 2024-01-22T18:47:40Z

ibc-apps/ics721-nft-transfer/types/src/packet.rs

-    }
-}
-
-impl TryFrom<RawPacketData> for PacketData {


Let's keep these TryFrom and From conversions as they should work fine. IIUC, we hit the “null” issue only if someone tries to obtain PacketData proto type from a JSON string. And, this actually stems from the fact that the .proto definitions and hence the Go compiler allow nullable fields, which shouldn't be the case. I noticed that ibc-go handles this by adding (gogoproto.nullable) = false to their .proto files.

Actually, I am thinking of keeping the nft-transfer types' crate complete in terms of providing needed conversions, in case, any other implementors need it.

* feat: establish ICS-721 boilerplate, ready for new additions (#1012) * chore: establish ics721 boilerplate, ready for new additions * nit * Implement ICS-721 NFT transfer (#1020) * WIP: add types and contexts * WIP: add events * WIP: implement modules * add send_transfer * add recv and refund handlers * add tests * fix send and recv * fix context and add tests * fix fmt * fix for CI * fix messages and serde * fix comments * Add (de)serialization tests for `DataValue`, `TokenUri`, and `ClassUri` types (#1027) * WIP: add types and contexts * WIP: add events * WIP: implement modules * add send_transfer * add recv and refund handlers * add tests * fix send and recv * fix context and add tests * fix fmt * fix for CI * fix messages and serde * fix comments * Stub out DataValue Borsh unit test * Add basic borsh (de)ser roundtrip tests * Add basic serde roundtrip tests for DataValue * Add json (de)serialization tests * Add roundtrip tests for TokenUri * Add roundtrip tests for ClassUri * Remove ignore statement on a test * Resolve clippy warning * Change packet data dummy json strings to use camel case * Configure nft-transfer app under std feature flag * Move cfg statement * Add nft-transfer feature * Add nft-transfer feature * Remove nft-transfer feature from default features * Remove `optional = true` from `http` dependency --------- Co-authored-by: yito88 <[email protected]> * fix: calculate trace hash from both class ID and token ID (#1032) * trace hash with class ID and token ID * add serde flag * Fix ClassData and TokenData encoding in NonFungiblePacketData (#1038) * fix encoding for ClassData and TokenData * fix Cargo.toml * Support ClassData and TokenData not according to ICS-721 spec (#1039) * skip validation, make some data optional * check the length of token_uri and token_data * fix to set TokenData and TokenUri at once * imp: add validate_basic method for PacketData * imp: allow any format for Data + define parse_as_ics721_data method * fmt and clippy * custom serde packet data with option * add a test * restore conversions --------- Co-authored-by: Farhad Shabani <[email protected]> * chore: add unclog * nit: fix docstrings --------- Co-authored-by: Yuji Ito <[email protected]> Co-authored-by: Sean Chen <[email protected]> Co-authored-by: yito88 <[email protected]>

yito88 added 3 commits January 14, 2024 00:20

skip validation, make some data optional

f12b240

check the length of token_uri and token_data

8036d96

fix to set TokenData and TokenUri at once

6eae3cb

Farhad-Shabani requested changes Jan 18, 2024

View reviewed changes

Farhad-Shabani added this to the 0.50.0 milestone Jan 18, 2024

Farhad-Shabani added 2 commits January 18, 2024 15:43

imp: add validate_basic method for PacketData

eeb2e4d

imp: allow any format for Data + define parse_as_ics721_data method

3edeaf6

fmt and clippy

82165da

yito88 added 2 commits January 22, 2024 00:31

custom serde packet data with option

f3806e5

add a test

5c9c52b

yito88 requested a review from Farhad-Shabani January 22, 2024 16:47

Farhad-Shabani reviewed Jan 22, 2024

View reviewed changes

restore conversions

d0a3499

yito88 requested a review from Farhad-Shabani January 22, 2024 21:33

Farhad-Shabani approved these changes Jan 22, 2024

View reviewed changes

Farhad-Shabani merged commit 1de2bd4 into cosmos:feat/ics721-impl Jan 22, 2024
14 checks passed

Farhad-Shabani mentioned this pull request Jan 23, 2024

feat: implement ICS-721 NFT transfer application #1053

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ClassData and TokenData not according to ICS-721 spec #1039

Support ClassData and TokenData not according to ICS-721 spec #1039

yito88 commented Jan 16, 2024 •

edited

Loading

codecov bot commented Jan 16, 2024 •

edited

Loading

Farhad-Shabani left a comment •

edited

Loading

yito88 commented Jan 18, 2024

Farhad-Shabani commented Jan 18, 2024

Farhad-Shabani left a comment

Farhad-Shabani Jan 22, 2024 •

edited

Loading

Support ClassData and TokenData not according to ICS-721 spec #1039

Support ClassData and TokenData not according to ICS-721 spec #1039

Conversation

yito88 commented Jan 16, 2024 • edited Loading

Description

PR author checklist:

Reviewer checklist:

codecov bot commented Jan 16, 2024 • edited Loading

Codecov Report

Farhad-Shabani left a comment • edited Loading

Choose a reason for hiding this comment

yito88 commented Jan 18, 2024

Farhad-Shabani commented Jan 18, 2024

Farhad-Shabani left a comment

Choose a reason for hiding this comment

Farhad-Shabani Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

yito88 commented Jan 16, 2024 •

edited

Loading

codecov bot commented Jan 16, 2024 •

edited

Loading

Farhad-Shabani left a comment •

edited

Loading

Farhad-Shabani Jan 22, 2024 •

edited

Loading